Skip to content

Fix backport CI#381

Merged
leofang merged 1 commit into
NVIDIA:mainfrom
leofang:fix_bp
Jan 12, 2025
Merged

Fix backport CI#381
leofang merged 1 commit into
NVIDIA:mainfrom
leofang:fix_bp

Conversation

@leofang

@leofang leofang commented Jan 12, 2025

Copy link
Copy Markdown
Member

@copy-pr-bot

copy-pr-bot Bot commented Jan 12, 2025

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@leofang leofang added the to-be-backported Trigger the bot to raise a backport PR upon merge label Jan 12, 2025
@leofang

leofang commented Jan 12, 2025

Copy link
Copy Markdown
Member Author

Let me add the backport label and admin-merge to test the backport CI. It should leave a comment in this PR to report an error.

@leofang
leofang merged commit 604fcac into NVIDIA:main Jan 12, 2025
@leofang
leofang deleted the fix_bp branch January 12, 2025 17:04
@github-actions

Copy link
Copy Markdown

Backport failed for 11.8.x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 11.8.x
git worktree add -d .worktree/backport-381-to-11.8.x origin/11.8.x
cd .worktree/backport-381-to-11.8.x
git switch --create backport-381-to-11.8.x
git cherry-pick -x 6c3e074e9ff8776ad99b44def15bc22ec7adada2

@leofang leofang added this to the cuda-python 12-next, 11-next milestone Jan 12, 2025
@leofang leofang added bug Something isn't working CI/CD CI/CD infrastructure P0 High priority - Must do! and removed to-be-backported Trigger the bot to raise a backport PR upon merge labels Jan 12, 2025
@github-actions

Copy link
Copy Markdown

Backport failed for 11.8.x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 11.8.x
git worktree add -d .worktree/backport-381-to-11.8.x origin/11.8.x
cd .worktree/backport-381-to-11.8.x
git switch --create backport-381-to-11.8.x
git cherry-pick -x 6c3e074e9ff8776ad99b44def15bc22ec7adada2

2 similar comments
@github-actions

Copy link
Copy Markdown

Backport failed for 11.8.x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 11.8.x
git worktree add -d .worktree/backport-381-to-11.8.x origin/11.8.x
cd .worktree/backport-381-to-11.8.x
git switch --create backport-381-to-11.8.x
git cherry-pick -x 6c3e074e9ff8776ad99b44def15bc22ec7adada2

@github-actions

Copy link
Copy Markdown

Backport failed for 11.8.x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 11.8.x
git worktree add -d .worktree/backport-381-to-11.8.x origin/11.8.x
cd .worktree/backport-381-to-11.8.x
git switch --create backport-381-to-11.8.x
git cherry-pick -x 6c3e074e9ff8776ad99b44def15bc22ec7adada2

@leofang leofang self-assigned this Jan 19, 2025
rparolin added a commit to rparolin/cuda-python that referenced this pull request Jul 21, 2026
It didn't actually guard the two lifetime fixes: the driver copies the path
during the set call (so the NVIDIA#379 latent UAF can't trigger), and a missing free
leaks silently (so NVIDIA#381 wouldn't fail the test either).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rparolin added a commit that referenced this pull request Jul 23, 2026
…p helper lifetime (#2399)

* Harden program-cache perms, binary-path resolution, and coredump helper lifetime

- cuda.core: create the on-disk program cache tree owner-only (0o700) and
  re-assert restrictive perms on POSIX, so cached device code cannot be read
  or planted by other local users regardless of the inherited umask.
- cuda.pathfinder: absolutize the resolved binary-utility path to honor the
  documented absolute-path contract; surface AddDllDirectory failures on
  Windows with GetLastError instead of silently swallowing them.
- cuda.bindings: retain the caller's bytes in _HelperCUcoredumpSettings so the
  borrowed pointer cannot outlive its backing buffer, and free the getter's
  1 KiB buffer in __dealloc__; clarify get_buffer_pointer's lifetime contract.

Adds regression tests for the cache permissions, path absolutization, and
coredump helper lifetime.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Simplify code comments on the hardening fixes

Shorten the explanatory comments to a line or two each and drop the internal
issue-number references (which don't resolve in this repo). No code changes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Remove agent_authored markers from the new tests

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Remove weak coredump buffer-lifetime test

It didn't actually guard the two lifetime fixes: the driver copies the path
during the set call (so the #379 latent UAF can't trigger), and a missing free
leaks silently (so #381 wouldn't fail the test either).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Silence bandit S103 on the intentionally world-writable test dir

The test pre-creates a 0o777 cache dir to verify the loader tightens it to
0o700; the permissive mask is the point of the test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test: expect absolutized binary path on Windows

The abspath change makes find_nvidia_binary_utility return a drive-qualified
path on Windows (C:\... ), so the three search-order tests must compare against
os.path.abspath(expected). No-op on Linux; fixes the Windows CI failures.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Raise on out-of-range c_int/c_byte kernel arguments instead of truncating

The c_int/c_byte param feeders silently narrowed an out-of-range Python int
(e.g. 2**32+5 -> 5). Range-check in the feeder and raise OverflowError; declare
feed() as 'except? -1' so Cython propagates the exception instead of swallowing
it. Addresses Glasswing V9.1 (leofang chose the raise option over matching
ctypes' silent wrap).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* param_packer: use PyLong_AsInt for c_int range check on Python 3.13+

Per Leo's review: on 3.13+ PyLong_AsInt converts and range-checks in one call
(raising OverflowError itself), so gate the manual INT_MIN/INT_MAX check to
pre-3.13 only. c_byte keeps the manual check (no 8-bit CPython equivalent).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* param_packer: unify c_int/c_byte range check via PyLong_AsLongAndOverflow

Per Leo's follow-up: use a single code path on all supported Pythons instead of
version-gating PyLong_AsInt. PyLong_AsLongAndOverflow flags out-of-long values
via its overflow out-param (no exception set), then we bounds-check the 32-bit
(c_int) / 8-bit (c_byte) target range and raise OverflowError. Drops the
PY_VERSION_HEX gate.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Split kernel-arg overflow change (#363) into its own PR (#2402)

The out-of-range c_int/c_byte -> OverflowError change is a distinct behavior
change; moved to a standalone PR so it can be reviewed separately from this
hardening batch. No functional change to the remaining hardening work.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Address review: scope cache perms to tmp, revert cybind docstring

Per @leofang's review on PR #2399:
- _file_stream.py: only the tmp/ staging dir is created owner-only (0o700).
  root/ and entries/ now inherit the umask / any pre-existing permissions so a
  deliberately shared kernel cache (e.g. group-writable on a compute cluster)
  keeps working. Dropped the post-mkdir chmod that would have re-tightened an
  existing shared directory. 0o700 in mkdir mode needs no chmod: umask only
  clears bits.
- _internal/utils.pyx: revert the get_buffer_pointer docstring change; that is
  cybind code and is out of scope for this PR.
- Tests updated to match: assert only tmp is 0o700, and that a pre-existing
  0o777 shared root is used as-is.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Document accepted security trade-off for shared program caches

Record the residual risk from narrowing the cache-permission hardening
(glasswing #359/#375) per PR #2399 review: committed entry files stay 0o600
(contents owner-only via mkstemp+os.replace), tmp/ is owner-only to protect
in-flight writes, but root/entries inherit the umask so shared caches keep
working. In a group-writable shared cache a group member could replace a
cached kernel; tamper-proofing that path needs load-time integrity checks and
is out of scope here.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Simplify cache permission comment; drop internal tool name

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Strip internal issue numbers from cache permission test docstring

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* pathfinder: use os.add_dll_directory() instead of raw kernel32 call

Per @leofang's review question: replace the direct ctypes
kernel32.AddDllDirectory() call with the stdlib os.add_dll_directory()
wrapper. Both call the same Win32 API, but the stdlib version drops the
hand-maintained argtypes/restype binding and reports failure as OSError
instead of a NULL cookie + GetLastError() check. Behavior is unchanged:
the returned handle is intentionally discarded (it has no finalizer, so
the directory stays on the search path for the process lifetime), and the
PATH mutation + failure warning are preserved.

Also strip an internal issue number from a test docstring.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working CI/CD CI/CD infrastructure P0 High priority - Must do!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant